Conversation
graebm
left a comment
graebm
left a comment
There was a problem hiding this comment.
Great job simplifying the export!
Would the the upload functionality be better off as a separate utility though?
| ] } | ||
| opentelemetry-proto = "0.26" | ||
| opentelemetry-otlp = "0.26" | ||
| opentelemetry-stdout = { version = "0.26", features = ["trace"] } |
There was a problem hiding this comment.
we don't use this. My bad for leaving it in
| opentelemetry-stdout = { version = "0.26", features = ["trace"] } |
|
|
||
| #[derive(Subcommand, Debug)] | ||
| enum Command { | ||
| RunBenchmark(RunArgs), |
There was a problem hiding this comment.
trivial/debatable: if we stick with this, I'd prefer a shorter name, since this is what we're doing 99% of the time
| RunBenchmark(RunArgs), | |
| Run(RunArgs), |
| #[derive(Subcommand, Debug)] | ||
| enum Command { |
There was a problem hiding this comment.
So, in the scripts that run all the benchmarks across the different language runners, it's assumed all runners take the same exact command line arguments (that's why the essential args are passed by position, so that it's easier to parse, regardless of language). See: https://github.com/awslabs/aws-crt-s3-benchmarks/?tab=readme-ov-file#run-a-benchmark
RUNNER_CMD S3_CLIENT WORKLOAD BUCKET REGION TARGET_THROUGHPUT
Fortunately RUNNER_CMD can be a list of strings (because it takes a lot of string to launch a java application), so we can make this work:
Edit this line:
aws-crt-s3-benchmarks/scripts/utils/build.py
Lines 233 to 234 in 508f630
to be like return [str(runner_src/'target/release/s3-benchrunner-rust', 'run-benchmark')]
| #[derive(Debug, clap::Args)] | ||
| #[command(flatten_help = true)] | ||
| struct UploadOtlpArgs { |
There was a problem hiding this comment.
It doesn't seem like this upload-otlp mode actually shares any code with the benchmark runner?
It seems simpler to just build this as a separate utility, rather than complicate the benchmark runner, unless you foresee a lot of shared functionality in the future?
2 participants
Description of changes:
Our telemetry output didn't quite match OTLP json format (e.g. unix timestamps are a string apparently). This prevents compatibility with other tooling in the observability space. Replace the manual conversion to use the actual generated protobuf types from
opentelemetry-rustand leverage the serde support from there directly.I also added a new subcommand
upload-otlpthat can process trace files and upload them to an OTLP endpoint for ingestion into an observability tool (e.g. OTel collector or independent viewer).TODO
I tried fixing up the graphing utils but something isn't quite right still. Need more time investigating.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.